fix(comment): say "waiting on checks" when a hold is not asking anyone to act - #10166
Merged
Conversation
…e to act #10116 stopped an unstable merge state from summoning a human: no manual-review label, not evicted from the merge train. The unified comment kept rendering that state as "manual review recommended" / "Suggested Action - Manual Review", so the comment and the disposition contradicted each other on the same PR -- the #5288 contradiction running the other way. `held` is reached from eight places and they split cleanly: an unsettled mergeable state is a WAIT that clears itself, while a guardrail hold, a durable GitHub merge refusal (#9862), an incomplete review, an explicit manual verdict, a close verdict on a never-closed author, and a real finding set all need a person. Only the wording branches, on one predicate defined beside the conditions in deriveUnifiedStatus that produce those cases -- re-deriving "is this actionable?" separately in the headline and the verdict box would be the same drift that let the comment and the disposition disagree to begin with. The status vocabulary is unchanged, so none of the four src readers or the existing assertions move. Deliberately NOT keyed on `ciState !== "passed"`. That vocabulary is passed|failed|unverified with no "pending" member, and `failed` already returns "blocked" -- so the only value that could reach the predicate via CI is `unverified`, which agent-actions.ts lists as a real manualHoldReason ("CI could not be verified"). An earlier draft included it and would have told a maintainer to stand down from a hold that was waiting on them; tsc rejecting a "pending" literal that does not exist is what surfaced it. Conservative by construction: any state the predicate does not recognise keeps today's Manual Review wording. Being wrongly told to look is a much smaller failure than being wrongly told not to. Closes #10117
Contributor
|
Superagent did not find any vulnerabilities or security issues in this PR. |
Contributor
|
Important 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏳ LoopOver is waiting…LoopOver has seen this pull request and is waiting on CI checks to finish before reviewing it. This comment will update once the review runs. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed · 🟨 Waiting |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10166 +/- ##
==========================================
- Coverage 92.21% 91.34% -0.88%
==========================================
Files 931 931
Lines 114071 114079 +8
Branches 27548 27554 +6
==========================================
- Hits 105188 104201 -987
- Misses 7584 8773 +1189
+ Partials 1299 1105 -194
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #10117
#10116 stopped an unstable merge state from summoning a human — no
manual-reviewlabel, not evicted from the merge train. The unified comment kept rendering that state as "manual review recommended" / "Suggested Action - Manual Review", so the comment and the disposition contradicted each other on the same PR. That is the #5288 contradiction running in reverse.heldhas eight sources and they split cleanlymanualverdictOnly the wording branches. The
UnifiedCommentStatusvocabulary is unchanged, so none of the foursrcreaders move and the existing assertions stand.The predicate lives beside the conditions in
deriveUnifiedStatusthat produce those cases, and both wording sites (headline and verdict box) read it. Re-deriving "is this actionable?" separately in each is the same drift that let the comment and the disposition disagree to begin with.The mistake worth recording
An earlier draft also treated
ciState !== "passed"as waiting. That vocabulary ispassed | failed | unverifiedwith no"pending"member, andfailedalready returns"blocked"— so the only value that could reach the predicate via CI wasunverified, whichagent-actions.tslists as a realmanualHoldReason("CI could not be verified"). That version would have told a maintainer to stand down from a hold that was waiting on them.tscrejecting a"pending"literal that does not exist is what surfaced it. The narrowed predicate is now merge-state only, and there is a dedicated regression test asserting unverified CI stays actionable.Verification
typecheckclean; 122 tests inunified-comment.test.ts; full sweep green.manual/closeverdictsThe first three matter more than the last: being wrongly told to look is noise, being wrongly told not to look is the failure that counts. The predicate is conservative by construction — any state it does not recognise keeps today's Manual Review wording.